fix: make DayValue serialization timezone stable#1114
Conversation
github-actions
Bot
added
bug
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1114 +/- ##
==========================================
- Coverage 58.36% 58.35% -0.02%
==========================================
Files 282 282
Lines 19405 19407 +2
==========================================
- Hits 11326 11324 -2
- Misses 7102 7104 +2
- Partials 977 979 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses timezone-dependent DayValue serialization to make YAML and string formatting stable across environments (fixing UT failures in western timezones, per issue #887).
Changes:
- Serialize
DayValuevia UTC (String()andMarshalYAML()) to avoid local-time date shifts. - Extend
TestDayValueYAMLwith a test case that forcestime.Localto a western timezone to catch regressions. - Remove an unused direct dependency (
github.com/sijms/go-ora/v2) and record the change inchanges/dev.md.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/flagext/day.go | Forces DayValue serialization paths to format using UTC for stability. |
| pkg/util/flagext/day_test.go | Adds a regression test covering western timezone behavior by overriding time.Local. |
| go.mod | Drops unused direct go-ora requirement. |
| go.sum | Removes corresponding go-ora checksum entries. |
| changes/dev.md | Adds an entry referencing issue #887. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Test UTC-stable string and YAML serialization in western timezones. | ||
| { | ||
| loc, err := time.LoadLocation("America/Los_Angeles") | ||
| if err != nil { |
There was a problem hiding this comment.
The comment says this block tests "UTC-stable string and YAML serialization", but the test only asserts YAML marshal/unmarshal output and never checks DayValue.String(). Either add an assertion for the String() output under the modified time.Local, or adjust the comment to only mention YAML serialization to keep the test intent accurate.
| type TestStruct struct { | ||
| Day *DayValue `yaml:"day"` | ||
| } | ||
| var testStruct TestStruct | ||
| testStruct.Day = &DayValue{} | ||
| require.NoError(t, testStruct.Day.Set("1985-06-02")) | ||
| expected := []byte(`day: "1985-06-02" | ||
| `) | ||
|
|
||
| actual, err := yaml.Marshal(testStruct) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, expected, actual) |
There was a problem hiding this comment.
This new timezone-specific YAML test duplicates the prior "pointers of DayValue" block almost line-for-line. Consider making the pointer test table-driven (e.g., parameterize the location) or extracting a small helper to reduce duplication and make future additions (more timezones) easier.
github-actions
Bot
added
bug
|
Consider whether any Copilot comments require action, and resolve the CI failures. |
|
Plz resolve the CI problem. |
What this PR does:
This PR make DayValue serialization timezone agnostic
Which issue(s) this PR fixes:
#887
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: